-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TS migration] Migrate WorkspaceReimburse Page #35399
[TS migration] Migrate WorkspaceReimburse Page #35399
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
src/components/Picker/BasePicker.tsx
Outdated
if (onInputChange) { | ||
onInputChange(inputValue); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (onInputChange) { | |
onInputChange(inputValue); | |
} | |
onInputChange?.(inputValue); |
src/components/Picker/BasePicker.tsx
Outdated
if (onInputChange) { | ||
onInputChange(inputValue, index); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (onInputChange) { | |
onInputChange(inputValue, index); | |
} | |
onInputChange?.(inputValue, index); |
|
||
const getUnitLabel = useCallback((value) => translate(`common.${value}`), [translate]); | ||
const getUnitLabel = useCallback((value: Unit) => translate(`common.${value}`), [translate]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const getUnitLabel = useCallback((value: Unit) => translate(`common.${value}`), [translate]); | |
const getUnitLabel = useCallback((value: Unit): string => translate(`common.${value}`), [translate]); |
const distanceCustomUnit = _.find(lodashGet(props.policy, 'customUnits', {}), (unit) => unit.name === CONST.CUSTOM_UNITS.NAME_DISTANCE); | ||
const distanceCustomRate = _.find(lodashGet(distanceCustomUnit, 'rates', {}), (rate) => rate.name === CONST.CUSTOM_UNITS.DEFAULT_RATE); | ||
const {translate, toLocaleDigit} = props; | ||
const [currentRatePerUnit, setCurrentRatePerUnit] = useState<string | undefined>(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const [currentRatePerUnit, setCurrentRatePerUnit] = useState<string | undefined>(''); | |
const [currentRatePerUnit, setCurrentRatePerUnit] = useState<string>(''); |
if (!customUnitRate) { | ||
return undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update getUnitRateValue
typing instead, it looks like it already can handle the case where customUnitRate
is undefined
if (!customUnitRate) { | |
return undefined; | |
} |
src/libs/actions/Policy.ts
Outdated
type UpdateCustomUnit = { | ||
customUnitID: string; | ||
name: string; | ||
attributes: Attributes; | ||
rates: Rate; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe I'd rename it to NewCustomUnit or CustomUnitUpdate
type UpdateCustomUnit = { | |
customUnitID: string; | |
name: string; | |
attributes: Attributes; | |
rates: Rate; | |
}; | |
type UpdateCustomUnit = CustomUnit & { | |
rates: Rate; | |
}; |
Navigation.goBack(ROUTES.WORKSPACE_REIMBURSE.getRoute(policy?.id ?? '')); | ||
}; | ||
|
||
const validate = (values: OnyxFormValuesFields<typeof ONYXKEYS.FORMS.WORKSPACE_RATE_AND_UNIT_FORM>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add return type
role={CONST.ROLE.PRESENTATION} | ||
inputID="rate" | ||
containerStyles={[styles.mt4]} | ||
defaultValue={distanceCustomRate ? PolicyUtils.getUnitRateValue(distanceCustomRate, toLocaleDigit) : ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, update getUnitRateValue typing instead
maxLength={12} | ||
/> | ||
|
||
<View style={[styles.mt4]}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<View style={[styles.mt4]}> | |
<View style={styles.mt4}> |
const hasVBA = achState === BankAccount.STATE.OPEN; | ||
const reimburseReceiptsUrl = `reports?policyID=${props.policy.id}&from=all&type=expense&showStates=Archived&isAdvancedFilterMode=true`; | ||
const isLoading = lodashGet(props.reimbursementAccount, 'isLoading', false); | ||
const reimburseReceiptsUrl = `reports?policyID=${policy?.id ?? ''}&from=all&type=expense&showStates=Archived&isAdvancedFilterMode=true`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract this url to CONST
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean add this into the CONST file?
6dcbdc6
to
4feb092
Compare
lastModified: PropTypes.number, | ||
}).isRequired, | ||
|
||
type WorkspaceReimburseViewOnyxProps = { | ||
/** From Onyx */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** From Onyx */ |
}, | ||
}; | ||
|
||
Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, parseInt(policy?.lastModified ?? '', 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update updateWorkspaceCustomUnitAndRate typing instead
Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, parseInt(policy?.lastModified ?? '', 2)); | |
Policy.updateWorkspaceCustomUnitAndRate(policy?.id ?? '', distanceCustomUnit, newCustomUnit, policy?.lastModified); |
if (!rateValueRegex.test(values.rate.toString()) || values.rate.toString() === 'Nan') { | ||
errors.rate = 'workspace.reimburse.invalidRateError'; | ||
} else if (NumberUtils.parseFloatAnyLocale(values.rate.toString()) <= 0) { | ||
errors.rate = 'workspace.reimburse.lowRateError'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have the old logic and update rate
type in values to string
instead
if (!rateValueRegex.test(values.rate.toString()) || values.rate.toString() === 'Nan') { | |
errors.rate = 'workspace.reimburse.invalidRateError'; | |
} else if (NumberUtils.parseFloatAnyLocale(values.rate.toString()) <= 0) { | |
errors.rate = 'workspace.reimburse.lowRateError'; | |
} | |
if (!rateValueRegex.test(values.rate) || values.rate === '') { | |
errors.rate = 'workspace.reimburse.invalidRateError'; | |
} else if (NumberUtils.parseFloatAnyLocale(values.rate) <= 0) { | |
errors.rate = 'workspace.reimburse.lowRateError'; | |
} |
# Conflicts: # src/ONYXKEYS.ts # src/pages/workspace/reimburse/WorkspaceRateAndUnitPage.js # src/pages/workspace/reimburse/WorkspaceReimbursePage.js # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/index.ts
# Conflicts: # src/ONYXKEYS.ts # src/libs/DistanceRequestUtils.ts # src/libs/actions/Policy.ts # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/Form.ts
@@ -1,6 +1,6 @@ | |||
type UpdateWorkspaceCustomUnitAndRateParams = { | |||
policyID: string; | |||
lastModified: number; | |||
lastModified?: number | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastModified?: number | string; | |
lastModified?: string; |
src/types/onyx/Form.ts
Outdated
|
||
type FormValueType = string | boolean | Date | OnyxCommon.Errors; | ||
type FormValueType = string | boolean | Date | number | OnyxCommon.Errors; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why number
it added, I've tried to remove it and ts check pass
<OfflineWithFeedback | ||
pendingAction={distanceCustomUnit?.pendingAction ?? distanceCustomRate?.pendingAction} | ||
shouldShowErrorMessages={false} | ||
> | ||
<MenuItemWithTopDescription | ||
title={currentRatePerUnit} | ||
description={translate('workspace.reimburse.trackDistanceRate')} | ||
shouldShowRightIcon | ||
onPress={() => Navigation.navigate(ROUTES.WORKSPACE_RATE_AND_UNIT.getRoute(policy?.id ?? ''))} | ||
wrapperStyle={[styles.mhn5, styles.wAuto]} | ||
brickRoadIndicator={(distanceCustomUnit?.errors ?? distanceCustomRate?.errors) && CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR} | ||
/> | ||
</OfflineWithFeedback> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was it added, the code looks really similar to the code block above, maybe you did a mistake during conflicts resolution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! It was badly conflicts merge! Thank you!
const currentCustomUnitRate = Object.values(distanceCustomUnit?.rates ?? {}).find((r) => r.name === CONST.CUSTOM_UNITS.DEFAULT_RATE); | ||
const unitID = distanceCustomUnit.customUnitID ?? ''; | ||
const unitName = distanceCustomUnit.name ?? ''; | ||
const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit) as number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we use Number(rateNumValue) instead of assertion?
const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit) as number; | |
const rateNumValue = PolicyUtils.getNumericValue(rate, toLocaleDigit); |
attributes: {unit}, | ||
rates: { | ||
...currentCustomUnitRate, | ||
rate: rateNumValue * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rate: rateNumValue * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, | |
rate: Number(rateNumValue) * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET, |
Will get to this one tomorrow. |
@ruben-rebelo Could you have a look at those merge conflicts? Thanks! |
@ruben-rebelo Seems there are some lint/typecheck errors. |
da439a9
to
34135a2
Compare
# Conflicts: # src/ONYXKEYS.ts # src/libs/PolicyUtils.ts # src/libs/actions/Policy.ts # src/pages/workspace/reimburse/WorkspaceReimburseView.tsx # src/types/onyx/Form.ts # src/types/onyx/Policy.ts # src/types/onyx/index.ts
@Ollyws It seems the conflicts were wrongly resolved. |
@ruben-rebelo Still got a typecheck error there if you could take a look, thanks! |
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25211 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@ruben-rebelo could you please run |
# Conflicts: # src/pages/workspace/reimburse/WorkspaceReimburseSection.tsx
@Ollyws Friendly bump |
Just waiting on @tylerkaraszewski's review. |
# Conflicts: # src/libs/DistanceRequestUtils.ts
Conflicts. |
# Conflicts: # src/libs/PolicyUtils.ts
@tylerkaraszewski Conflicts resolved. Thanks! |
Tests are currently failing. |
Also, conflicts. |
# Conflicts: # src/libs/PolicyUtils.ts
@tylerkaraszewski PR updated and conflicts resolved |
Performance tests aren't currently passing. |
@ruben-rebelo Could you check this? Thanks! |
@tylerkaraszewski It needed merge with main. Tests are now passing. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Details
[TS migration] Migrate WorkspaceReimburse page to TypeScript
Fixed Issues
$ #25211
PROPOSAL: N/A
Tests
Test WorkspaceReimburse Pages:
Offline tests
N/A
QA Steps
Same as in the Tests section.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android-native.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
macos-web.mp4
MacOS: Desktop
macos-desktop.mov